fix(sdk): bound AsyncExecutor.close() so it cannot hang forever - #4548

Requesting changes: this is a mitigation, not a fundamental fix. Bounding AsyncExecutor.close() to 30s replaces "hang forever" with "block all conversations for 30s". That's a shorter wedge, but still a wedge — and serial 30s stalls accumulate under load.

The root cause is that close() runs under the global _lifecycle_lock (in delete_conversation), so even a bounded hang blocks every other conversation. See the ready-for-dev issue #4569 for the fundamental solution (per-conversation locks).

This PR can likely be closed as superseded once #4569 is implemented — if close() can only block its own conversation, a timeout becomes a safety net rather than a critical fix.

Requesting changes: this is a mitigation, not a fundamental fix. Bounding AsyncExecutor.close() to 30s replaces "hang forever" with "block all conversations for 30s". That's a shorter wedge, but still a wedge — and serial 30s stalls accumulate under load.

The root cause is that close() runs under the global _lifecycle_lock (in delete_conversation), so even a bounded hang blocks every other conversation. See the ready-for-dev issue #4569 for the fundamental solution (per-conversation locks).

This PR can likely be closed as superseded once #4569 is implemented — if close() can only block its own conversation, a timeout becomes a safety net rather than a critical fix.

Requesting changes: this is a mitigation, not a fundamental fix. Bounding AsyncExecutor.close() to 30s replaces "hang forever" with "block all conversations for 30s". That's a shorter wedge, but still a wedge — and serial 30s stalls accumulate under load.

The root cause is that close() runs under the global _lifecycle_lock (in delete_conversation), so even a bounded hang blocks every other conversation. See the ready-for-dev issue #4569 for the fundamental solution (per-conversation locks).

This PR can likely be closed as superseded once #4569 is implemented — if close() can only block its own conversation, a timeout becomes a safety net rather than a critical fix.

Thanks for the thorough reproduction and for addressing the unbounded shutdown path. The direction is useful as a last-resort containment mechanism, especially now that per-conversation lifecycle locks limit the blast radius. Before merging, please address these safety requirements:

1. Document the semantics precisely. This is bounded, best-effort shutdown—not guaranteed cleanup. After the timeout, the portal/helper/worker threads and their resources may still be alive. Please make that explicit in the API docstring, warning, and PR description.

2. Make abandonment observable. The timeout warning should clearly state that shutdown was abandoned and resources may remain active. Include enough context to identify the executor/owner, the timeout, and that cancellation was attempted.

3. Preserve useful failure information. Avoid reducing shutdown failures to only str(exception). Log the exception with traceback/context where appropriate, while keeping teardown non-raising. Broad exception handling is acceptable for destructor-safe cleanup only if the failure remains diagnosable.

4. Keep production shutdown bounded. Audit callers and confirm that no production path passes timeout=None; do not reintroduce the original unbounded behavior through the compatibility option.

5. Reconsider the default timeout. Thirty seconds is inherited from the browser cleanup timeout, but it may be too long for lifecycle teardown. Please justify the value with normal shutdown measurements or choose a shorter shutdown-specific default.

6. Add regression coverage. Tests should cover cancellation of cancellable tasks, timeout/abandonment of uncancellable synchronous work, idempotent close, never-started portals, observable timeout diagnostics, and shutdown exceptions. The timeout-path test should explicitly document that background threads may remain.

7. Track the remaining limitation. Please link or note #4598: arbitrary synchronous code cannot be forcibly interrupted from Python threads, so this change contains the caller-side wedge but does not eliminate possible zombie threads.

This review was created by an AI agent (OpenHands) on behalf of the user.

Approving: the rebased commits address the safety review items. Bounded best-effort semantics are documented; abandonment is observable (owner+timeout+cancellation-attempted) and the helper thread is named for traceability; teardown failures log with exc_info; all production callers use the 10s default (none pass timeout=None); #4598 is linked for the un-interruptible-thread limitation. #4570 (per-conversation locks) is merged, so a stuck close() now only blocks its own conversation — this is a safety net, not the load-bearing fix. CI green.